Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize types in implicitScope too #20138

Closed
wants to merge 1 commit into from

Conversation

smarter
Copy link
Member

@smarter smarter commented Apr 9, 2024

This is needed for the same reason than the normalization in computeIScope added by 90c3fbd.

IMO, it's already very hard to figure out where .normalize should be used, but the fact that it sometimes is only needed under separate compilation is really error-prone and leads to very hard to debug issues. Ideally we should deal with the same types under separate and join compilation.

While I'm at it, I added one defensively in addPath since I can't tell whether it is needed or not.

Fixes #20136.

This is needed for the same reason than the normalization in `computeIScope`
added by 90c3fbd.

IMO, it's already very hard to figure out where `.normalize` should be used,
but the fact that it sometimes is only needed under separate compilation is
really error-prone and leads to very hard to debug issues. Ideally we should
deal with the same types under separate and join compilation.

While I'm at it, I added one defensively in `addPath` since I can't tell
whether it is needed or not.

Fixes scala#20136.
Copy link
Contributor

@EugeneFlesselle EugeneFlesselle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed our changes with Deferred here, might want to rebase on them

@@ -724,7 +724,7 @@ trait ImplicitRunInfo:
companions ++= iscopeRefs(t.underlying)
end addCompanions

def addPath(pre: Type): Unit = pre.dealias match
def addPath(pre: Type): Unit = pre.dealias.normalized match
Copy link
Contributor

@EugeneFlesselle EugeneFlesselle Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you still think we need this given our earlier findings ?

@smarter
Copy link
Member Author

smarter commented Apr 11, 2024

Superceded by #20147.

@smarter smarter closed this Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recent regression in interaction between match types and implicit conversion under separate compilation
2 participants